refactor: deprecate pytz in favor of builtin zoneinfo#2757
Conversation
351a3d7 to
812539d
Compare
|
@JoLo90 thanks for taking this on. Am I correct in thinking that, other that removing Since the intent is to remove |
|
@cwhanse |
7793ea6 to
2c4be54
Compare
cwhanse
left a comment
There was a problem hiding this comment.
Two other items:
- Thanks for showing how to fix the CI issues with Windows tests. But let's not do that fix in this PR; should be it's own PR. Please revert changes to pytest.yml
- There are two Warnings in the iPython notebook with a local path. It would help if those messages were suppressed somehow. Also, can you rerun the notebook with the execution counter reset?
|
|
||
| The read-only ``pytz`` attribute will stay in sync with any changes made | ||
| using ``tz``. | ||
| The ``pytz`` attribute is deprecated. Use ``tz`` property instead. |
There was a problem hiding this comment.
| The ``pytz`` attribute is deprecated. Use ``tz`` property instead. | |
| The ``pytz`` attribute is deprecated. Use ``tz`` instead. |
|
|
||
| Deprecations | ||
| ~~~~~~~~~~~~ | ||
| * Deprecated ``Location.pytz`` attribute. Use ``Location.tz`` property instead. |
There was a problem hiding this comment.
| * Deprecated ``Location.pytz`` attribute. Use ``Location.tz`` property instead. | |
| * ``Location.pytz`` is deprecated. Use ``Location.tz`` instead. |
2c4be54 to
5e45255
Compare
|
@cwhanse I implemented your comments and removed the local path warning in the jupyter notebook (note that these local paths appear in the other notebooks). The CI issues with Windows seem to be solved, so I won't open a new PR. |
5e45255 to
ff907e1
Compare
There was a problem hiding this comment.
I'm no expert on zoneinfo... I confess I didn't know it existed until this PR, but following @cwhanse's request I have had a quick scan over.
Besides a conventional point on the whatsnew file, and I am unsure about the diff on the notebook, I see the relevant instances of pytz have been replaced and a warning has been added, we have planned a removal version (0.17.0), docs are updated, and all checks pass... so this LGTM.
One question/note: do we need to update pyproject.toml to remove pytz as a dependency? (in 0.17.0)
I would also add on the side that we do need to update #2396 to keep track of the scheduled removal too. @echedey-ls ?
e5c98a6 to
12e3158
Compare
echedey-ls
left a comment
There was a problem hiding this comment.
My first round of comments and thoughts here.
Thanks @cwhanse for the ping and @RDaxini for the deprecation tracker reminder.
@JoLo90 , can you configure your environment to not force push an squashed commit of all your changes? We will squash and merge later, it can help us review to see the progress and small changes between one review and another.
There was a problem hiding this comment.
I'm okay with keeping just the bare minimal changes to be done in here, but since it's mostly noise, I would revert all changes to it - I won't review this file if there are so many lines changed.
It's obvious that you didn't know it, but these files are no longer maintained nor we expect them to work with latest pvlib versions (I think, please any maintainer correct me if I'm wrong).
There was a problem hiding this comment.
@echedey-ls ok I'll just modify the line where the outdated pytz property was used. If it doesn't work anymore, maybe we should delete them.
There was a problem hiding this comment.
For the ipython files, whatever is easiest from this point onwards. Since the PR submitter updated the workbook, I don't think we need to revert changes.
And I don't recall whether we decided (or just talked about) to not update the notebooks.
| """ | ||
| if tz: | ||
| tzname = pytz.timezone(f'Etc/GMT{-tz:+d}') | ||
| tzname = zoneinfo.ZoneInfo(f'Etc/GMT{-tz:+d}') # noqa: E231 |
There was a problem hiding this comment.
I can't see the commit history, did you add # noqa: E231 because flake8 was complaining? I may be wrong, but sintax in this line shouldn't raise any flake8 warning. I can't reproduce locally in a reasonable amount of time (flake8 5.0.4 is tech debt at this point...).
There was a problem hiding this comment.
@echedey-ls yes it was a false positive, flake8 insisted on putting a space after the :
af4c3a6 to
e85ab50
Compare
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
Co-authored-by: Echedey Luis <80125792+echedey-ls@users.noreply.github.com>
e85ab50 to
15e60b0
Compare
Description
Replaces pytz with Python's built-in zoneinfo module for all timezone handling in pvlib.
There is an ongoing issue with micromamba that has been solved thanks to mamba-org/setup-micromamba#306. See commit [ci: update micromamba setup and version].
Changes
Checklist
docs/sphinx/source/referencefor API changes.docs/sphinx/source/whatsnewfor all changes. Includes link to the GitHub Issue with:issue:`num`or this Pull Request with:pull:`num`. Includes contributor name and/or GitHub username (link with:ghuser:`user`).remote-data) and Milestone are assigned to the Pull Request and linked Issue.